Skip to content

Keep DropdownMenu open after selection#604

Open
riko111 wants to merge 19 commits intoDroidKaigi:mainfrom
riko111:main
Open

Keep DropdownMenu open after selection#604
riko111 wants to merge 19 commits intoDroidKaigi:mainfrom
riko111:main

Conversation

@riko111
Copy link

@riko111 riko111 commented Sep 8, 2025

Issue

Overview (Required)

  • Keep DropdownMenu open after item selection to allow multiple selections

Links

Movie (Optional)

Before After
2025-09-08.1.42.31.mov
20250908_164950.mp4

@riko111 riko111 requested a review from a team as a code owner September 8, 2025 08:20
@riko111 riko111 requested review from mikanIchinose and removed request for a team September 8, 2025 08:20
@github-actions
Copy link

github-actions bot commented Sep 8, 2025

Snapshot diff report

File name Image
io.github.droidkaigi
.confsched.sessions.
SearchScreenTest - S
earchScreen - when s
erver is operational
- after loading - w
hen filter language
chip click - when cl
ick English - it sho
uld selected English
_compare.png
io.github.droidkaigi
.confsched.sessions.
SearchScreenTest - S
earchScreen - when s
erver is operational
- after loading - i
t should no timetabl
e items are displaye
d_compare.png
io.github.droidkaigi
.confsched.sessions.
SearchScreenTest - S
earchScreen - when s
erver is operational
- after loading - w
hen filter day chip
click - it should sh
ow drop down menu_co
mpare.png
io.github.droidkaigi
.confsched.sessions.
SearchScreenTest - S
earchScreen - when s
erver is operational
- after loading - w
hen filter category
chip click - when cl
ick App Architecture
en - it should sele
cted App Architectur
e en_compare.png
io.github.droidkaigi
.confsched.sessions.
SearchScreenTest - S
earchScreen - when s
erver is operational
- after loading - w
hen filter day chip
click - when click 2
- it should selecte
d 2_compare.png
SearchScreenPreview_
1_compare.png
io.github.droidkaigi
.confsched.sessions.
SearchScreenTest - S
earchScreen - when s
erver is operational
- after loading - w
hen filter language
chip click - when cl
ick Japanese - it sh
ould selected Japane
se_compare.png
io.github.droidkaigi
.confsched.sessions.
SearchScreenTest - S
earchScreen - when s
erver is operational
- after loading - i
nput search word to
TextField - it shoul
d show search word a
nd filtered items_co
mpare.png
io.github.droidkaigi
.confsched.sessions.
SearchScreenTest - S
earchScreen - when s
erver is operational
- after loading - w
hen filter category
chip click - when cl
ick Jetpack Compose
en - it should selec
ted Jetpack Compose
en_compare.png
io.github.droidkaigi
.confsched.sessions.
SearchScreenTest - S
earchScreen - when s
erver is operational
- after loading - w
hen filter day chip
click - when click 1
- it should selecte
d 1_compare.png
io.github.droidkaigi
.confsched.sessions.
SearchScreenTest - S
earchScreen - when s
erver is operational
- after loading - w
hen filter category
chip click - it shou
ld show drop down me
nu_compare.png
SearchScreenPreview_
0_compare.png
io.github.droidkaigi
.confsched.sessions.
SearchScreenTest - S
earchScreen - when s
erver is operational
- after loading - w
hen filter language
chip click - it shou
ld show drop down me
nu_compare.png
io.github.droidkaigi
.confsched.sessions.
SearchScreenTest - S
earchScreen - when s
erver is operational
- after loading - w
hen filter category
chip click - when cl
ick Other en - it sh
ould selected Other
en_compare.png

Copy link
Contributor

@kitakkun kitakkun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank you. 👍🏻

@riko111 riko111 self-assigned this Sep 8, 2025
@kitakkun
Copy link
Contributor

kitakkun commented Sep 8, 2025

This PR has already been approved, but we’ve noticed some UX concerns and are currently discussing the direction within the team.

To explain briefly, with the current implementation, after selecting an item from the dropdown, the user needs to manually tap outside the menu each time to close it. There’s a concern that this extra step may not provide a good user experience.

Also, in typical use cases, users are unlikely to select multiple session categories at once, so the existing behavior might actually be more preferable from a UX perspective.

Sorry for the inconvenience, and thank you for your patience. We’ll reach out again once we’ve settled on a direction. 🙏🏻

@kitakkun
Copy link
Contributor

kitakkun commented Sep 8, 2025

@riko111
Thanks so much for your work on this! After discussing with our team, we think it might be best if this feature is implemented with the following behavior:

  • When a dropdown menu has multiple options, long-pressing a specific item could enter multi-selection mode.
    • We suggest adding Haptic feedback to indicate when entering that mode.
  • Once in multi-selection mode, selecting other items shouldn’t close the dropdown menu.

Would you be able to update your implementation to match these specifications? Thanks in advance!

@riko111
Copy link
Author

riko111 commented Sep 8, 2025

I'll do my best.

@riko111
Copy link
Author

riko111 commented Sep 9, 2025

This PR updates the DropdownMenu behavior to support the requested multi-selection mode specifically for category items:

  • Long-pressing a category item now enters multi-selection mode.
  • Haptic feedback is triggered when entering multi-selection mode.
  • While in multi-selection mode, selecting additional category items does not close the dropdown menu.

These changes align the implementation with the discussed specifications and improve usability when selecting multiple categories.

20250909-103047.mp4

By the way, do we still need the session type dropdown list? It might be unnecessary, especially since it doesn't exist in the iPhone version.

@riko111 riko111 requested a review from kitakkun September 9, 2025 01:44
fun SearchFilterRow(
filters: SearchScreenUiState.Filters,
onFilterToggle: (SearchScreenEvent.Filter) -> Unit,
onFilterLongPress: ((SearchScreenEvent.Filter) -> Unit)? = null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed onFilterLongPress is always null 👀. Maybe we could consider removing this argument?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out, I hadn’t noticed it.
I’ll remove the parameter along with the comment below.

Comment on lines 157 to 173
modifier = if (onItemLongPress != null) {
Modifier.combinedClickable(
onLongClick = {
if (selectedItems.isNotEmpty()) {
onItemLongPress(selectedItems.first())
}
},
) {
scope.launch {
withFrameNanos {
keyboardController?.hide()
}
}.invokeOnCompletion { expanded = true }
}
} else {
Modifier
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since onItemLongPress is always null, maybe we could consider removing these lines of code.

}
},
onLongClick = {
if (!isMultiSelectMode) isMultiSelectMode = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this if check might not be necessary. Maybe we could just set isMultiSelectMode = true directly?

Comment on lines 220 to 221
.padding(horizontal = 12.dp, vertical = 8.dp)
.combinedClickable(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be better to invoke combinedClickable before applying the padding modifier. That way, the ripple effect would behave more properly.

image

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that’s really helpful and I learned something!

Comment on lines 237 to 239
if (item in selectedItems) {
Icon(Icons.Default.Check, contentDescription = null, modifier = Modifier.padding(end = 12.dp))
}
Copy link
Contributor

@kitakkun kitakkun Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hiding the icon entirely when unselected might cause the view width to shift compared to the existing behavior, which could feel janky. Maybe controlling the alpha instead would be smoother.

Screen_recording_20250909_150111.mp4

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Along with this, I also locked the chip width while the menu is open, so the dropdown list position no longer shifts horizontally.

@kitakkun
Copy link
Contributor

kitakkun commented Sep 9, 2025

By the way, do we still need the session type dropdown list? It might be unnecessary, especially since it doesn't exist in the iPhone version.

There’s a corresponding issue opened for this: #500.

…dth while menu is open

- Fade out unselected check icons instead of removing them to avoid width shifts
- Lock the chip width while the dropdown menu is expanded so the anchor position stays stable
…dth while menu is open

- Fade out unselected check icons instead of removing them to avoid width shifts
- Lock the chip width while the dropdown menu is expanded so the anchor position stays stable
@riko111
Copy link
Author

riko111 commented Sep 9, 2025

Addressed the review feedback and updated the implementation accordingly.

screen-20250909-172113.mp4

Comment on lines -150 to -155
if (selectedItems.isNotEmpty()) {
Icon(
imageVector = Icons.Default.Check,
contentDescription = null,
)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate your effort with this change 🙏🏻, but for this checkmark, we don’t need to control its visibility via alpha. It’s fine to keep the original display logic as it was, since this introduces an unintended visual change.

image

Comment on lines 144 to 157
modifier = Modifier
.then(
if (lockedChipWidthDp != null) {
Modifier.width(lockedChipWidthDp!!)
} else {
Modifier
},
)
.onGloballyPositioned { cords ->
currentChipWidthPx = cords.size.width
if (expanded && lockedChipWidthDp == null) {
lockedChipWidthDp = with(density) { currentChipWidthPx.toDp() }
}
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand the issue of the dropdown menu moving during selection, this implementation is somewhat tricky and unnecessarily increases complexity, so I’d like to avoid that.

It seems that placing the DropdownMenu directly at the same hierarchy level as the FilterChip causes the FilterChip to become the anchor for the menu, which is why this problem occurs.

Wrapping the DropdownMenu in a Box could be a good solution. It might seem a bit unusual, but this way the dropdown’s anchor becomes a stable, empty Box, which keeps its position consistent.

Box {
    FilterChip()
    // stable anchor point for DropdownMenu
    Box {
        DropdownMenu()
    }
}

Comment on lines 182 to 228
DropdownMenuItem(
modifier = Modifier.testTag(DropdownFilterChipTestTagPrefix.plus(item)),
leadingIcon = {
if (selectedItems.contains(item)) {
Icon(
imageVector = Icons.Default.Check,
contentDescription = null,
)
}
},
text = { Text(itemLabel(item)) },
onClick = {
onItemSelected(item)
expanded = false
},
)
Row(
modifier = Modifier
.testTag(DropdownFilterChipTestTagPrefix.plus(item))
.fillMaxWidth()
.heightIn(min = 48.dp)
.combinedClickable(
onClick = {
onItemSelected(item)
if (!isMultiSelectMode) {
expanded = false
}
},
onLongClick = {
isMultiSelectMode = true
haptics.performHapticFeedback(HapticFeedbackType.LongPress)
onItemSelected(item)
},
)
.padding(horizontal = 12.dp, vertical = 8.dp),
verticalAlignment = Alignment.CenterVertically,
) {
Icon(
Icons.Default.Check,
contentDescription = null,
modifier = Modifier
.padding(end = 12.dp)
.alpha(if (selectedItems.contains(item)) 1f else 0f),
)
Text(itemLabel(item), style = MaterialTheme.typography.bodyLarge)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a slightly unconventional approach, but I came up with a way to make DropdownMenuItem respond to long presses. I’m not claiming this method is objectively better, so whether to adopt it is entirely up to you.

By implementing it like this, you can enable long-clicks on DropdownMenuItem without creating a custom menu item. The idea is to overlay a box of the same size on top of the menu item and handle click events there:

Box(
    modifier = Modifier.testTag(DropdownFilterChipTestTagPrefix.plus(item))
) {
    DropdownMenuItem(
        leadingIcon = {
            if (selectedItems.contains(item)) {
                Icon(
                    imageVector = Icons.Default.Check,
                    contentDescription = null,
                )
            }
        },
        text = { Text(itemLabel(item)) },
        onClick = { /* actual click events are handled in the overlayed Box */ },
    )
    Box(
        Modifier
            .matchParentSize()
            .combinedClickable(
                onClick = {
                    onItemSelected(item)
                    if (!isMultiSelectMode) {
                        expanded = false
                    }
                },
                onLongClick = {
                    isMultiSelectMode = true
                    haptics.performHapticFeedback(HapticFeedbackType.LongPress)
                    onItemSelected(item)
                },
            )
    )
}

In short, the top Box intercepts both normal and long clicks, so you don’t need to customize DropdownMenuItem itself.

What do you think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I first tried writing it the way you suggested, but I couldn’t get it to work, so I ended up using the current approach.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But with the other feedback helping me tidy up the code, I feel more confident it will work this time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not sure why, but it seems that adding the following Modifier to the DropdownMenuItem not to the outer Box allows the existing tests to pass without issues:

modifier = Modifier.testTag(DropdownFilterChipTestTagPrefix.plus(item))

items: List<T>,
itemLabel: (T) -> String,
onItemSelected: (T) -> Unit,
onMultiSelectFinished: (List<T>) -> Unit = {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifying default values for lambda parameters has some downsides: it can make it easy to forget to provide a value, and at a glance it’s not always clear whether a value is actually being passed. Therefore, unless there’s a specific reason, I’d prefer to remove default empty lambdas.

@kitakkun
Copy link
Contributor

kitakkun commented Sep 9, 2025

I’ve left a few comments—when you have a chance, please take a look 🙏🏻

I realize they’re a bit nitpicky, but they touch on aspects that are actually important for UX and code maintainability. I appreciate your understanding!

@riko111
Copy link
Author

riko111 commented Sep 9, 2025

I really appreciate you reviewing multiple times during the busy day before the first day. After reorganizing the code, the approach that didn’t work at first ended up working.

@riko111 riko111 requested a review from kitakkun September 11, 2025 09:30
@kitakkun
Copy link
Contributor

@riko111
Since I got a review request, I just wanted to ask—maybe there’s a way to resolve the test failure with the approach I suggested in my previous comment?

@kitakkun
Copy link
Contributor

Just a quick comment.

If it’s only the changes for Issue #500, it could be merged independently, so feel free to consider doing that!

@riko111
Copy link
Author

riko111 commented Sep 11, 2025

Sorry, I think I mistakenly sent a review request. I haven’t had a chance to look into it yet, so please withdraw it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add ability to select multiple filters at once 【Proposal】Eliminate the session type field from the search screen.

2 participants